Top recent trading pages #7710

zutigrm · 2023-10-17T17:45:30Z

Summary

Addresses issue:

Create “Top recent trending pages” key metric widget tile #7603

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2023-10-17T17:54:08Z

Build files for 643d395 are ready:

Download development build
Download production build

github-actions · 2023-10-17T17:55:15Z

Size Change: +641 B (0%)

Total Size: 1.41 MB

Filename	Size	Change
`./dist/assets/js/googlesitekit-activation-********************.js`	22.4 kB	+5 B (0%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	54.1 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	32.9 kB	+15 B (0%)
`./dist/assets/js/googlesitekit-api-********************.js`	9.85 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.1 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	18 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.19 kB	+7 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	23.5 kB	+4 B (0%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	67.2 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	83.7 kB	+138 B (0%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	107 kB	+11 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-********************.js`	87 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	75.6 kB	+424 B (+1%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.3 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	20.7 kB	+5 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	62.6 kB	-18 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	31 kB	+4 B (0%)
`./dist/assets/js/googlesitekit-settings-********************.js`	47.6 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-splash-********************.js`	65.6 kB	+5 B (0%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	40.4 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-vendor-********************.js`	316 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	30.4 kB	-7 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	62.7 kB	+6 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	51.6 kB
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.4 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.58 kB
`./dist/assets/js/29-********************.js`	2.8 kB
`./dist/assets/js/30-********************.js`	2.28 kB
`./dist/assets/js/31-********************.js`	3.72 kB
`./dist/assets/js/32-********************.js`	930 B
`./dist/assets/js/33-********************.js`	888 B
`./dist/assets/js/34-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	769 B
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.7 kB
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10 kB
`./dist/assets/js/googlesitekit-data-********************.js`	2.18 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B
`./dist/assets/js/runtime-********************.js`	1.3 kB

_{compressed-size-action}

tofumatt

I think there's some duplication we can remove in the story file for this component, as well as some tweaks to the function that handles the report options.

I also have a question about the renaming, which I've pinged @nfmohit about as well, but if you know the reason @zutigrm just let me know 🙂

tofumatt · 2023-10-17T19:57:51Z

assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPages.js

+const getReportOptions = () => {
+	const today = new Date();
+	const todayDateString = getDateString( today );
+
+	const yesterday = getPreviousDate( todayDateString, 1 );
+	const twoDaysAgo = getPreviousDate( todayDateString, 2 );
+	const threeDaysAgo = getPreviousDate( todayDateString, 3 );
+
+	const dates = {
+		startDate: threeDaysAgo,
+		endDate: yesterday,
+	};
+
+	const reportOptions = {
+		...dates,
+		dimensions: [ 'pagePath' ],
+		dimensionFilters: {
+			'customEvent:googlesitekit_post_date': {
+				filterType: 'inListFilter',
+				value: [
+					yesterday.replace( /-/g, '' ),
+					twoDaysAgo.replace( /-/g, '' ),
+					threeDaysAgo.replace( /-/g, '' ),
+				],
+			},
+		},
+		metrics: [ { name: 'screenPageViews' } ],
+		orderby: [
+			{
+				metric: {
+					metricName: 'screenPageViews',
+				},
+				desc: true,
+			},
+		],
+		limit: 3,
+	};
+
+	return [ reportOptions, dates ];
+};
+function TopRecentTrendingPages( { Widget } ) {
+	const viewOnlyDashboard = useViewOnly();
+
+	const dates = getReportOptions()[ 1 ];
+
+	const reportOptions = getReportOptions()[ 0 ];


Why is this function outside of the component? We tend to place functions that a component uses inside them.

Given this is only for this component, let's move it inside the component.

In fact, you call this function from inside the component multiple times but it's probably worth calling it once and storing the results… or even just declaring these variables all directly in the component. I don't see why we wouldn't do that 🤔

Suggested change

const getReportOptions = () => {

const today = new Date();

const todayDateString = getDateString( today );

const yesterday = getPreviousDate( todayDateString, 1 );

const twoDaysAgo = getPreviousDate( todayDateString, 2 );

const threeDaysAgo = getPreviousDate( todayDateString, 3 );

const dates = {

startDate: threeDaysAgo,

endDate: yesterday,

};

const reportOptions = {

...dates,

dimensions: [ 'pagePath' ],

dimensionFilters: {

'customEvent:googlesitekit_post_date': {

filterType: 'inListFilter',

value: [

yesterday.replace( /-/g, '' ),

twoDaysAgo.replace( /-/g, '' ),

threeDaysAgo.replace( /-/g, '' ),

],

},

},

metrics: [ { name: 'screenPageViews' } ],

orderby: [

{

metric: {

metricName: 'screenPageViews',

},

desc: true,

},

],

limit: 3,

};

return [ reportOptions, dates ];

};

function TopRecentTrendingPages( { Widget } ) {

const viewOnlyDashboard = useViewOnly();

const dates = getReportOptions()[ 1 ];

const reportOptions = getReportOptions()[ 0 ];

function TopRecentTrendingPages( { Widget } ) {

const viewOnlyDashboard = useViewOnly();

const today = new Date();

const todayDateString = getDateString( today );

const yesterday = getPreviousDate( todayDateString, 1 );

const twoDaysAgo = getPreviousDate( todayDateString, 2 );

const threeDaysAgo = getPreviousDate( todayDateString, 3 );

const dates = {

startDate: threeDaysAgo,

endDate: yesterday,

};

const reportOptions = {

...dates,

dimensions: [ 'pagePath' ],

dimensionFilters: {

'customEvent:googlesitekit_post_date': {

filterType: 'inListFilter',

value: [

yesterday.replace( /-/g, '' ),

twoDaysAgo.replace( /-/g, '' ),

threeDaysAgo.replace( /-/g, '' ),

],

},

},

metrics: [ { name: 'screenPageViews' } ],

orderby: [

{

metric: {

metricName: 'screenPageViews',

},

desc: true,

},

],

limit: 3,

};

tofumatt · 2023-10-17T20:02:05Z

assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPages.js

+		dimensions:
+			KEY_METRICS_WIDGETS[ KM_ANALYTICS_TOP_RECENT_TRENDING_PAGES ]
+				.requiredCustomDimensions?.[ 0 ],
+		reportOptions: getReportOptions()[ 0 ],


Oh, I see, this is why we have the function outside the component. That would be worth noting in a comment at the top of the file, as it's unclear otherwise and unexpected 😄

Additionally, let's have the function return an object so the reportOptions and dates are keys rather than using numerical indexes for the returned values. getReportOptions().reportOptions is more readable than getReportOptions()[ 0 ].

Even better might be getReportOptions() only returning the report options, given the dates are simple and consistent/hardcoded we could either specify them in both the function and the component, or have a separate getDateRange() function. 🤔

tofumatt · 2023-10-17T20:02:45Z

assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPages.stories.js

+const today = new Date();
+const todayDateString = getDateString( today );
+
+const yesterday = getPreviousDate( todayDateString, 1 );
+const twoDaysAgo = getPreviousDate( todayDateString, 2 );
+const threeDaysAgo = getPreviousDate( todayDateString, 3 );
+
+const reportOptions = {
+	startDate: threeDaysAgo,
+	endDate: yesterday,
+	dimensions: [ 'pagePath' ],
+	dimensionFilters: {
+		'customEvent:googlesitekit_post_date': {
+			filterType: 'inListFilter',
+			value: [
+				yesterday.replace( /-/g, '' ),
+				twoDaysAgo.replace( /-/g, '' ),
+				threeDaysAgo.replace( /-/g, '' ),
+			],
+		},
+	},
+	metrics: [ { name: 'screenPageViews' } ],
+	orderby: [
+		{
+			metric: {
+				metricName: 'screenPageViews',
+			},
+			desc: true,
+		},
+	],
+	limit: 3,
+};


These should be exported from the component's file so the story can reuse them, rather than specifying them again here.

@tofumatt Great point, thanks. I extracted it into a new dir, so other custom dimensions dependant widgets can also use it.

tofumatt · 2023-10-17T20:06:34Z

assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPages.js

@@ -0,0 +1,208 @@
+/**
+ * TopCitiesWidget component.


This title doesn't match the filename/component name, by that raises a separate question: why rename this anyway? With this new filename it's the only KMW in that folder I can see whose component name/filename doesn't end with Widget. I don't know what the motivation for that is, as I didn't see it in the ACs of the issue and it was mentioned in the IB but I'm not sure why.

cc @nfmohit who wrote the IB, maybe you can shed some light on the renaming here? 😄

Hi @tofumatt. The naming in the IB was an oversight. Since all widget names end with Widget, we should also do the same here. Thanks for checking!

aaemnnosttv · 2023-10-17T20:12:38Z

@zutigrm This branch also has merge conflicts that need resolving.

…ent.

tofumatt

Looks like the refactoring here introduced a bug, but I can fix it then get this merged 👍🏻

tofumatt · 2023-10-18T10:56:33Z

assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPagesWidget.js


 function TopRecentTrendingPagesWidget( { Widget } ) {
+	const viewOnlyDashboard = useViewOnly();
+
+	const dates = getReportOptions()[ 1 ];


This function is no longer returning an array (or the dates at all) so this will fail 🤔

techanvil · 2023-10-18T16:24:29Z

assets/js/modules/index.test.js

-		if ( ! components.length ) {
+
+		// Filter out the custom-dimensions-report-options directory
+		const filteredComponents = components.filter(


Hey @zutigrm, are these changes to this file needed or could they have been introduced by accident? When I remove them and run the test suite locally, it still passes...

Hey @techanvil, they are not now, as the folder structure is removed in favour of date/report options functions being in the same file. I will remove that from the test in my next issue

Sounds good, thanks @zutigrm!

zutigrm added 15 commits October 9, 2023 13:45

Add new widget to key-metrics-widgets list.

be32be3

Add KM_ANALYTICS_TOP_RECENT_TRENDING_PAGES const.

129c614

Add TopRecentTrendingPages widget.

953152c

Add stories file.

bc8e74d

Fix In_List_Filter dimension value.

f9c9374

Include Top recent trending pages widget in the list.

44fe2c7

Add rows to the metric tile.

60f2c84

Change vlaues to value in dimensionFilters.

bf959ad

Move top recent trending pages under newsKeyMetric flag.

a2b14c6

Update validate shared dimensions with custom dimension.

0fe5b48

Sync with develop.

8f5f9f4

Remove unused widget included with sync.

3891d00

Move reportOptions into function so it can be used in HOC.

5b0f3f3

Update stories.

65c0154

Update VRT images.

970353f

tofumatt requested changes Oct 17, 2023

View reviewed changes

zutigrm added 7 commits October 18, 2023 09:11

Rename to TopRecentTrendingPagesWidget.

e36b7af

Extract report options into separate file.

e2a6ccb

Update report options and dates to re-use function and renamed compon…

178b307

…ent.

Use report options from file and component rename.

a8debe9

Update VRT to adjust to component name change.

0eb8339

Resolve merge conflict.

b60c50c

Fix failing test.

a90b482

tofumatt requested changes Oct 18, 2023

View reviewed changes

Use correct date values in widget.

643d395

tofumatt approved these changes Oct 18, 2023

View reviewed changes

tofumatt merged commit b2012d4 into develop Oct 18, 2023
23 of 24 checks passed

tofumatt deleted the enhancement/7603-top-recent-trading-pages branch October 18, 2023 13:44

techanvil reviewed Oct 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top recent trading pages #7710

Top recent trading pages #7710

zutigrm commented Oct 17, 2023 •

edited by tofumatt

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

tofumatt left a comment

tofumatt Oct 17, 2023

tofumatt Oct 17, 2023

tofumatt Oct 17, 2023

zutigrm Oct 18, 2023

tofumatt Oct 17, 2023

nfmohit Oct 17, 2023

aaemnnosttv commented Oct 17, 2023

tofumatt left a comment

tofumatt Oct 18, 2023

techanvil Oct 18, 2023 •

edited

Loading

zutigrm Oct 24, 2023

techanvil Oct 24, 2023

Top recent trading pages #7710

Top recent trading pages #7710

Conversation

zutigrm commented Oct 17, 2023 • edited by tofumatt Loading

Summary

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Oct 17, 2023 • edited Loading

github-actions bot commented Oct 17, 2023 • edited Loading

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv commented Oct 17, 2023

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zutigrm commented Oct 17, 2023 •

edited by tofumatt

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

techanvil Oct 18, 2023 •

edited

Loading